-
Notifications
You must be signed in to change notification settings - Fork 2.8k
19.0 Estate tutorial (VIKRI) #1078
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 19.0
Are you sure you want to change the base?
Conversation
barracudapps
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments here and there. Can you also please review your PR title and description following our guidelines (https://www.odoo.com/documentation/19.0/contributing/development/git_guidelines.html)?
A clear commit message is really important to describe what you did and why you did it.
Completed till chapter 7 many to one
Added new tags model to understand many2many
| postcode = fields.Char(string="Postcode") | ||
| date_availability = fields.Datetime(string="Date Availability", default=_set_default_start_date(), copy=False) | ||
| date_availability = fields.Datetime( | ||
| string="Date Availability", default=_set_default_start_date(), copy=False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a trailing coma here so if someone has to add a new parameter on a new line, that person won't have to change your line (and change the git history)
real_estate/data/res_estate_data.xml
Outdated
| <field name="description">Description 3</field> | ||
| <field name="postcode">1040</field> | ||
| <field name="date_availability" eval="DateTime.now()" /> | ||
| <field name="date_availability" eval="DateTime.now()"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date_availability is already pre-defined in your model with your default value
| state = fields.Selection( | ||
| string="State", | ||
| selection=[ | ||
| ("new", "New"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use double quotes (") only for non-technical strings.
Please check all your files
| access_estate_property,access.estate.property,model_estate_property,base.group_user,1,1,1,1 | ||
| access_estate_property_type,access.estate.property.type,model_estate_property_type,base.group_user,1,1,1,1 | ||
| access_estate_property_type,access.estate.property.type,model_estate_property_type,base.group_user,1,1,1,1 | ||
| access_estate_property_tags,access.estate.property.tags,model_estate_property_tags,base.group_user,1,1,1,1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget Unix conventions on text files' structure based on a line definition (cf. https://peps.python.org/pep-0008/ and https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206)
Check other files too
real_estate/views/estate_menu.xml
Outdated
| <menuitem | ||
| id="estate_menu_action" | ||
| action="test_model_action" | ||
| parent="estate_menu_root" | ||
| sequence="1" | ||
| /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can keep this on a single line. It's not always easy to know when to write multiple lines but the basic rule is that you try to optimize readability AND number of lines
| <?xml version="1.0"?> | ||
| <odoo> | ||
| <!-- Root menuitem action which will help us to see the default action window--> | ||
| <!-- Root menuitem action which will help us to see the default action window--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is useless as the majority of the developers will easily know what your code is doing
Check the style guide and resolve the issues
1.create new model and view for offers 2.complete chapter 7
1. Sold and cancel button and actions added 2. Accept and reject button added with actions for offers
…n estate_property.py
expected and offer price must be strictly positive Selling price must be positive Tag name should be unique
1.use status bar widget to show the state of the property 2. add default sorting order for the models 3. add colors for tag model 4. add invisible in the needed places as mentioned in the task 5. make the offer and tag editable in list view
barracudapps
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the module !
Still some small suggestions but LGTM 👍
You can mark as ready (kind of message you'll get in R&D but cannot be applied here)
| def action_set_sold(self): | ||
| for record in self: | ||
| if record.state == 'cancelled': | ||
| raise UserError("This property cannot be sold.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark your strings as translatable using self.env._ method
| _name_uniq = models.Constraint( | ||
| 'unique (name)', | ||
| "This tag is already available", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably fits a single line
|
|
||
|
|
||
| class EstatePropertyType(models.Model): | ||
| _name = "estate.property.type" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are tech strings 😉
| <field name="name">estate.property.list</field> | ||
| <field name="model">estate.property</field> | ||
| <field name="arch" type="xml"> | ||
| <list string="Estate" decoration-success="state=='offered' or state=='accepted'" decoration-bf="state=='accepted'" decoration-muted="state=='sold'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using in to check multiple possible values (instead of or)
| <field name="postcode"/> | ||
| <filter name="available" string="Available" domain="['|', ('state', '=', 'new'), ('state', '=', 'offer_received')]"/> | ||
| <filter name="inactive" string="Archived" domain="[('active', '=', False)]"/> | ||
| <field name="living_area" string="Living Area (sqm)" filter_domain="[('living_area', '>=', self)]"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer this for stability
| <field name="living_area" string="Living Area (sqm)" filter_domain="[('living_area', '>=', self)]"/> | |
| <field name="living_area" string="Living Area (sqm)" filter_domain="[('living_area', '>=', self)]"/> |

Here I have created new add-on for on-boarding tutorial project
what I have done?